Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

abhinav · 2022-03-08T17:45:03Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

The gopackagesdriver relies on the "stdliblist" action provided by rules_go.
The stdliblist action fails on Go 1.18 with the following error:

external/go_sdk/src/crypto/elliptic/p256_asm.go:24:12: pattern p256_asm_table.bin: cannot embed irregular file p256_asm_table.bin

We see this failure because Bazel builds a symlink tree of the GOROOT to run
go list with. However, since CL 380475, the Go standard library uses
go:embed to embed a file in crypto/elliptic, but go:embed does not
support symlinks.

Fix this by having stdliblist copy the relevant portions of the GOROOT to
run go list with. This matches what the stdlib action does.

Which issues(s) does this PR fix?

Fixes #3080

Other notes for review

I've updated the existing stdlib test to also depend on the output of
stdliblist, and verified that that fails with Go 1.18rc1 without this change.

The stdliblist operation that the gopackagesdriver relies on fails on Go 1.18rc1 with the following error: ``` external/go_sdk/src/crypto/elliptic/p256_asm.go:24:12: pattern p256_asm_table.bin: cannot embed irregular file p256_asm_table.bin ``` We see this failure because Bazel builds a symlink tree of the GOROOT run `go list` with. However, since [CL 380475][1], the Go standard library uses `go:embed` to embed a file in `crypto/elliptic`, but `go:embed` does not support symlinks. [1]: https://go.dev/cl/380475 Fix this by having stdliblist copy the relevant portions of the GOROOT to run `go list` with. This matches [what the stdlib action does][2]. [2]: https://github.com/bazelbuild/rules_go/blob/e9a7054ff11a520e3b8aceb76a3ba44bb8da4c94/go/tools/builders/stdlib.go#L54-L57 Resolves #3080

Add a dependency on `GoStdLib._list_json` to the stdlib test. This ensures that we can successfully build the JSON data needed by the gopackagesdriver.

abhinav · 2022-03-08T21:07:07Z

This isn't quite right. Specifically, the output paths are not right:

Before:

  "GoFiles": [
    "__BAZEL_OUTPUT_BASE__/external/go_sdk/src/archive/tar/common.go",
    "__BAZEL_OUTPUT_BASE__/external/go_sdk/src/archive/tar/format.go",
    "__BAZEL_OUTPUT_BASE__/external/go_sdk/src/archive/tar/reader.go",

Now:

  "GoFiles": [
    "__BAZEL_OUTPUT_BASE__/src/archive/tar/common.go",
    "__BAZEL_OUTPUT_BASE__/src/archive/tar/format.go",
    "__BAZEL_OUTPUT_BASE__/src/archive/tar/reader.go",

These are missing the "external/go_sdk".

Switching to WIP while I investigate.

The prior version of this fix was incomplete because it generated incorrect relative paths. For example, before a path would be: __BAZEL_OUTPUT_BASE__/external/go_sdk/src/archive/tar/common.go But with the prior version of this change. __BAZEL_OUTPUT_BASE__/src/archive/tar/common.go It would drop the external/go_sdk from the path because the flattening logic in stdliblist makes these relative to the execRoot. We cannot overwrite external/go_sdk in execRoot because that's a path controlled by Bazel. Instead, create a parallel external/go_sdk under a new directory "root", and flatten paths relative to that.

abhinav · 2022-03-08T22:36:11Z

Fixed. The solution is a bit janky: we have to create a copy of external/go_sdk under a $newdir/external/go_sdk and flatten the paths relative to $newdir to get the same output. This isn't ideal, but I'm not sure how else to get Bazel to create copies of the file instead of symlinks.

linzhp · 2022-03-26T04:03:59Z

go/tools/builders/stdliblist.go

+// cloneRoot returns the new root directory and the new GOROOT we should run
+// under.
+func cloneRoot(execRoot, goroot string) (newRoot string, newGoroot string, err error) {
+	relativeGoroot, err := filepath.Rel(abs(execRoot), abs(goroot))


If you add args.add("-sdk", go.sdk.root_file.dirname) to here:

rules_go/go/private/actions/stdlib.bzl

Lines 56 to 57 in d606ba2

args = go.builder_args(go, "stdliblist")

args.add("-out", out)

Then you can get a relative path of goroot from execRoot with goenv.sdk.

linzhp · 2022-03-26T04:24:06Z

go/tools/builders/stdliblist.go

@@ -158,19 +195,22 @@ func stdliblist(args []string) error {
 		return err
 	}

+	execRoot, goroot, err := cloneRoot(".", os.Getenv("GOROOT"))


"execRoot" is a special name in Bazel. It means the "The working directory for all actions" (https://bazel.build/docs/output_directories). Because you copied the symlink tree to to a different place, it's no longer the actual execRoot. Please rename it to something like "newBase" or "cloneBase". There are several variables/arguments in this file called "execRoot". After this pull request, they are no longer the actual execroot. Please also renaming all of them.

linzhp · 2022-03-26T04:26:38Z

go/tools/builders/stdliblist.go

+		return "", "", err
+	}
+
+	if err := replicate(goroot, newGoroot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil {


Do you need to replicate pkg/tool?

linzhp · 2022-03-26T04:40:25Z

go/tools/builders/stdliblist.go

+		return "", "", fmt.Errorf("GOROOT %q is not relative to execution root %q: %v", goroot, execRoot)
+	}
+
+	newRoot = filepath.Join(execRoot, "root")


I am a bit nervous about writing things to execRoot. It's supposed to contain a symlink tree intended to be read-only. This may have unexpected result if there happen to be a root directory under execRoot. Can you copy it to ioutil.TempDir?

linzhp · 2022-03-26T05:26:24Z

I am also thinking about how to write tests to cover stdliblist. The go_test will need a data = "@go_sdk//:files" and access it using: filepath.Join(os.Getenv("TEST_SRCDIR"), "go_sdk"). So there is no longer an "external/go_sdk" section in the abs path. In order for stdliblist to work in such test, you will also need to replace this function with something like filepath.Join("__BAZEL_OUTPUT_BASE__/external", p[:strings.Index("/go_sdk/")])

linzhp · 2022-05-19T14:57:31Z

Superseded by #3157

abhinav added 2 commits March 8, 2022 17:30

test/stdlib: Depend on _list_json

c6a2b7c

Add a dependency on `GoStdLib._list_json` to the stdlib test. This ensures that we can successfully build the JSON data needed by the gopackagesdriver.

linzhp self-requested a review March 8, 2022 17:53

abhinav changed the title ~~Fix gopackagesdriver for Go 1.18 by replicating stdlib~~ WIP: Fix gopackagesdriver for Go 1.18 by replicating stdlib Mar 8, 2022

abhinav changed the title ~~WIP: Fix gopackagesdriver for Go 1.18 by replicating stdlib~~ Fix gopackagesdriver for Go 1.18 by replicating stdlib Mar 8, 2022

abhinav mentioned this pull request Mar 18, 2022

Go package driver doesn't work with Go 1.18 #3080

Closed

linzhp requested changes Mar 26, 2022

View reviewed changes

linzhp self-assigned this Mar 26, 2022

linzhp mentioned this pull request Apr 5, 2022

enable windows/arm64 go build with bazel #3072

Merged

This was referenced May 13, 2022

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

Closed

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3157

Merged

linzhp closed this May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

abhinav commented Mar 8, 2022

abhinav commented Mar 8, 2022 •

edited

Loading

abhinav commented Mar 8, 2022

linzhp Mar 26, 2022

linzhp Mar 26, 2022

linzhp Mar 26, 2022

linzhp Mar 26, 2022

linzhp commented Mar 26, 2022

linzhp commented May 19, 2022

	args = go.builder_args(go, "stdliblist")
	args.add("-out", out)

Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

Conversation

abhinav commented Mar 8, 2022

abhinav commented Mar 8, 2022 • edited Loading

abhinav commented Mar 8, 2022

linzhp Mar 26, 2022

Choose a reason for hiding this comment

linzhp Mar 26, 2022

Choose a reason for hiding this comment

linzhp Mar 26, 2022

Choose a reason for hiding this comment

linzhp Mar 26, 2022

Choose a reason for hiding this comment

linzhp commented Mar 26, 2022

linzhp commented May 19, 2022

abhinav commented Mar 8, 2022 •

edited

Loading